Problem: test transactions can't be saved and reused #1575
Conversation
mmsqe
requested review from
yihuang and
JayT106
and removed request for
a team
|
Warning Rate limit exceeded@yihuang has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 38 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes enhance transaction handling and node initialization within the Changes
Assessment against linked issues
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1575 +/- ##
=======================================
Coverage 15.24% 15.24%
=======================================
Files 67 67
Lines 4874 4874
=======================================
Hits 743 743
Misses 4037 4037
Partials 94 94 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- testground/benchmark/benchmark/stateless.py (3 hunks)
Additional comments not posted (1)
testground/benchmark/benchmark/stateless.py (1)
363-363: LGTM!The usage of
calculate_tpsfunction in thedump_block_statsfunction is correct. Thecalculate_tpsfunction is already calculating the average TPS for a range of blocks as suggested in the past review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
testground/benchmark/benchmark/stateless.py (1)
355-370: LGTM!The function is using the
calculate_tpsfunction as suggested in the past review comment.Consider adding more statistics such as average TPS and median TPS.
The function can be further improved by adding more statistics such as average TPS and median TPS as suggested in the past review comment.
Apply this diff to add more statistics:
top_tps = sorted(tps_list, reverse=True)[:3] print("top_tps", top_tps) + print("average_tps", sum(tps_list) / len(tps_list)) + print("median_tps", sorted(tps_list)[len(tps_list) // 2])
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- testground/benchmark/benchmark/stateless.py (3 hunks)
Additional comments not posted (3)
testground/benchmark/benchmark/stateless.py (3)
316-326: The past review comment is still valid and applicable:coderabbitai[bot]: Consider simplifying the function using the
datetimemodule.The function can be simplified by using the
datetimemodule to parse and format the timestamp.Apply this diff to refactor the function:
-def truncate_fractional_seconds(timestamp): - ( - date_time_part, - _, - _, - ) = timestamp.partition("Z") - if "." in date_time_part: - date_time_part, fractional_part = date_time_part.split(".") - fractional_part = fractional_part[:6] - return f"{date_time_part}.{fractional_part}Z" - return timestamp +from datetime import datetime + +def truncate_fractional_seconds(timestamp): + dt = datetime.strptime(timestamp, "%Y-%m-%dT%H:%M:%S.%fZ") + return dt.strftime("%Y-%m-%dT%H:%M:%S.%fZ")
329-354: The past review comment is still valid and applicable:coderabbitai[bot]: LGTM!
The function logic is correct and it handles the case where there is no valid time difference.
Consider optimizing the function by caching the previous block's timestamp.
The function can be optimized by caching the previous block's timestamp to avoid redundant API calls.
Apply this diff to optimize the function:
+prev_timestamp = None + def calculate_tps(start_height, end_height): total_txs = 0 total_time = 0 for height in range(start_height + 1, end_height): blk = block(height) timestamp = blk["result"]["block"]["header"]["time"] - prev_blk = block(height - 1) - txs = len(prev_blk["result"]["block"]["data"]["txs"]) + if prev_timestamp is None: + prev_blk = block(height - 1) + prev_timestamp = prev_blk["result"]["block"]["header"]["time"] + txs = len(blk["result"]["block"]["data"]["txs"]) if txs > 0: - prev_timestamp = prev_blk["result"]["block"]["header"]["time"] time_format = "%Y-%m-%dT%H:%M:%S.%fZ" current_time, previous_time = ( datetime.strptime(truncate_fractional_seconds(ts), time_format) for ts in (timestamp, prev_timestamp) ) diff = (current_time - previous_time).total_seconds() total_time += diff total_txs += txs + prev_timestamp = timestamp if total_time > 0: average_tps = total_txs / total_time return average_tps else: return 0
49-50: LGTM!The function is correctly parsing the
optionsargument as JSON if it is a string.
mmsqe
changed the title
mmsqe
force-pushed
the
top_tps
branch
2 times, most recently
from
394e3f0 to
4f7a658
Compare
Solution: - extract standalone changes from crypto-org-chain#1575 - use a simple and deterministic way to generate test accounts
* Problem: bulk-add-genesis-account is not used in testground Solution: - extract standalone changes from #1575 - use a simple and deterministic way to generate test accounts * fix * fix * fix coins * fix comment * multi coins * cleanup * fix py lint --------- Co-authored-by: mmsqe <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- testground/benchmark/benchmark/sendtx.py (1 hunks)
- testground/benchmark/benchmark/stateless.py (9 hunks)
Files skipped from review as they are similar to previous changes (1)
- testground/benchmark/benchmark/stateless.py
Additional context used
GitHub Check: Lint python
testground/benchmark/benchmark/sendtx.py
[failure] 65-65:
./testground/benchmark/benchmark/sendtx.py:65:1: BLK100 Black would make changes.
[failure] 65-65:
./testground/benchmark/benchmark/sendtx.py:65:1: W293 blank line contains whitespace
[failure] 76-76:
./testground/benchmark/benchmark/sendtx.py:76:89: E501 line too long (92 > 88 characters)
Additional comments not posted (3)
testground/benchmark/benchmark/sendtx.py (3)
15-17: LGTM!The
sendtxfunction has been simplified to handle a single transaction, which improves readability and maintainability. The logic is correct, and the implementation is accurate.
20-40: LGTM!The
generate_loadfunction has been modified to read transactions from a file, which allows for more flexible load testing scenarios. The use of aThreadPoolExecutorenables concurrent transaction sending, improving performance. The logic is correct, and the implementation is accurate.
43-50: LGTM!The
create_txsfunction encapsulates the logic for creating transactions, which improves code modularity and reusability. The function generates a list of raw transaction hex strings for a given account, which can be used for efficient load testing. The logic is correct, and the implementation is accurate.
Solution: - add tps calculation, extract from crypto-org-chain#1575
Solution: - add tps calculation, extract from crypto-org-chain#1575
* Problem: testground block stats don't calculate tps directly Solution: - add tps calculation, extract from #1575 * cleanup * Update testground/benchmark/benchmark/stats.py Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: yihuang <[email protected]> * skip block 1 --------- Signed-off-by: yihuang <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: mmsqe <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- testground/benchmark/benchmark/stateless.py (8 hunks)
Additional context used
Ruff
testground/benchmark/benchmark/stateless.py
9-9:
datetime.datetimeimported but unusedRemove unused import:
datetime.datetime(F401)
GitHub Check: Lint python
testground/benchmark/benchmark/stateless.py
[failure] 9-9:
./testground/benchmark/benchmark/stateless.py:9:1: F401 'datetime.datetime' imported but unused
Additional comments not posted (6)
testground/benchmark/benchmark/stateless.py (6)
26-26: LGTM!The import statement is correct and the imported functions are used in the code.
109-117: LGTM!The additional arguments passed to the
init_node_localfunction are correct and used to configure the node initialization process.
126-126: LGTM!The
num_accountsandnum_txsarguments passed to theinit_node_localfunction for fullnodes are correct and used to configure the fullnode initialization process.
189-197: LGTM!The
num_workersandsleepoptions added to therunfunction are correct and used to configure the load generation process. The default values are appropriate.
Line range hint
212-256: LGTM!The code changes in the
runfunction are correct:
- The
do_runfunction is called with thenum_workersandsleeparguments to configure the load generation process.- The output data collection and saving process is correct and only applies to validator nodes.
- The
generate_loadfunction is called with thenum_workersandsleeparguments to generate load.
344-360: LGTM!The code changes in the
init_node_localfunction are correct:
- The
num_txsandvalidator_generate_loadparameters are added to configure the transaction generation process during node initialization.- The
gen_txsfunction is called with the correct arguments to generate transactions.- The transaction generation is conditionally executed based on the
validator_generate_loadflag.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (3)
testground/benchmark/benchmark/sendtx.py (3)
28-32: Consider renaming the function towrite_tx.The function name
async_write_txsuggests that it is intended to be used asynchronously, but there are no async/await keywords used within the function. To avoid confusion, consider renaming the function towrite_txsince it doesn't contain any asynchronous operations.-def async_write_tx(account, tx, file_path): +def write_tx(account, tx, file_path): raw_tx = account.sign_transaction(tx).rawTransaction with open(file_path, "w") as file: file.write(ujson.dumps([raw_tx.hex()]))
34-37: Fix the typo in the function name.There is a typo in the function name
get_txs_foler. It should beget_txs_folderfor clarity and consistency.-def get_txs_foler(outdir: Path, global_seq): +def get_txs_folder(outdir: Path, global_seq): folder = outdir / TMP_TXS_FOLDER / str(global_seq) folder.mkdir(parents=True, exist_ok=True) return folder
59-65: Update the function name based on the previous suggestion.Update the function name
get_txs_folertoget_txs_folderbased on the previous suggestion to fix the typo.def read_txs(outdir: Path, global_seq): txs = [] - folder = get_txs_foler(outdir, global_seq) + folder = get_txs_folder(outdir, global_seq) for file in folder.glob("*.json"): with open(file, "r") as f: tx_data = ujson.load(f) txs.append(tx_data[0]) return txs
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- testground/benchmark/benchmark/sendtx.py (3 hunks)
- testground/benchmark/benchmark/stateless.py (7 hunks)
Files skipped from review as they are similar to previous changes (1)
- testground/benchmark/benchmark/stateless.py
Additional comments not posted (5)
testground/benchmark/benchmark/sendtx.py (5)
3-3: LGTM!The import statement for the
Pathclass from thepathlibmodule is correctly placed at the top of the file. ThePathclass is a useful utility for working with file paths in a cross-platform manner.
8-8: LGTM!The import statement for the
gen_accountfunction from the.utilsmodule is correctly placed at the top of the file. Thegen_accountfunction is likely used to generate test accounts for transaction preparation.
14-14: LGTM!The constant
TMP_TXS_FOLDERis defined correctly, following the naming convention for constants in Python. It specifies the folder name for storing temporary transaction files.
85-85: LGTM!Creating an
aiohttp.TCPConnectorwith a connection limit specified by theCONNECTION_POOL_SIZEconstant is a good practice. It helps control the maximum number of concurrent connections to the server and provides a centralized configuration option.
Line range hint
1-90: Overall assessment of the file:The file follows a modular and asynchronous approach for preparing, reading, and sending transactions, which is a good design choice. The separation of concerns into different functions improves code organization and reusability. The use of
aiohttpandasynciofor asynchronous processing enables concurrent handling of transactions, leading to better performance.The file also utilizes constants for configuration values such as
GAS_PRICE,CHAIN_ID,LOCAL_JSON_RPC, andCONNECTION_POOL_SIZE, making it easier to modify and maintain these values in a centralized manner.However, there are a couple of areas that could be improved:
The error handling in the
prepare_txsfunction could be enhanced by logging errors with more details or retrying the signing process in case of transient failures. This would provide better visibility into any issues encountered during transaction preparation.The typo in the function name
get_txs_folershould be fixed toget_txs_folderfor clarity and consistency with the intended functionality.Overall, the file demonstrates a solid structure and design, with room for minor improvements in error
Solution: - support saving test transactions to files and reuse to speedup local test Co-authored-by: mmsqe <[email protected]>
yihuang
changed the title
diff --git a/testground/benchmark/benchmark/stateless.py b/testground/benchmark/benchmark/stateless.py
index 5e4db258..4fb54d0e 100644
--- a/testground/benchmark/benchmark/stateless.py
+++ b/testground/benchmark/benchmark/stateless.py
@@ -6,6 +6,7 @@ import socket
import subprocess
import tarfile
import tempfile
+import threading
import time
from pathlib import Path
from typing import List
@@ -220,12 +221,26 @@ def _gen_txs(
num_txs: int = 1000,
):
outdir = Path(outdir)
- for global_seq in range(nodes):
+
+ # create dir in advance to avoid conflict in parallel task
+ (outdir / transaction.TXS_DIR).mkdir(parents=True, exist_ok=True)
+
+ def job(global_seq: int):
print("generating", num_accounts * num_txs, "txs for node", global_seq)
txs = transaction.gen(global_seq, num_accounts, num_txs)
transaction.save(txs, outdir, global_seq)
print("saved", len(txs), "txs for node", global_seq)
+ threads = [
+ threading.Thread(target=job, args=(global_seq,)) for global_seq in range(nodes)
+ ]
+
+ for t in threads:
+ t.start()
+
+ for t in threads:
+ t.join()
+
def do_run(
datadir: Path, home: Path, cronosd: str, group: str, global_seq: int, cfg: dictI actually tested this parallel implementation, CPU usage never pass |
|
yes, I thought need generate tx_{acc_i}_{tx_i}.json file in parallel avoid regenerate when changing num_accts and num_txs |
I think we just backup the whole |
Solution:
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
Closes: #1573
and
diable validator_generate_load
to run
PR Checklist:
make)make test)go fmt)golangci-lint run)go list -json -m all | nancy sleuth)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes